-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Order updates with signals #639
Order updates with signals #639
Conversation
e55caaa
to
6bc3787
Compare
sdk-core-protos/protos/local/temporal/sdk/core/workflow_activation/workflow_activation.proto
Outdated
Show resolved
Hide resolved
// | ||
// The downside of this reordering is that a signal or update handler may not observe that some | ||
// other event had already happened (ex: an activity completed) when it is first invoked, though it | ||
// will subsequently when workflow routines are driven. Core only does this reordering to make life |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a signal or update handler may not observe that some other event had already happened (ex: an activity completed) when it is first invoked
OK, this makes sense: the activity completion is in the "other" category and therefore happens after the update handler is executed.
though it will subsequently when workflow routines are driven.
Struggling with this. Does "driving workflow routines" happen when jobs in the "other" category are handled? If so, the update handler has stopped executing by that point hasn't it, so in what sense does it (when not executing) observe something that it didn't when it first executed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does (though, per what I said in that other issue, ideally it happens at the end of processing all the jobs rather than inline with it).
The handler counts as one of the workflow routines, and so is again allowed to advance at the end of job processing. At that point, it would see the activity as completed, if it hadn't already exited.
476bc3e
to
5ed70eb
Compare
5ed70eb
to
6ccb489
Compare
What happens when update handlers start local activities? Is the order of the local activities preserved, in the sense that they are moved before other jobs, and they respect the signals/update order? |
signal_name: "1".to_string(), | ||
..Default::default() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the application code also uses the signal_name "1", do we need to reserve some signal names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm? This is just test code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry...Duh...
The two things aren't related. If an update handler starts local activities, the job for that update must have came either in that activation, or some previous one to start the handler. There are no jobs for the local activities until they resolve, and then they come in along with any other jobs, and any signals and activities will be present before them (but they'd necessarily be invocations of some other handler, or brand new invocations of the same handler. IE: Not related). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python still has old code that is doing sorting of jobs. I have opened temporalio/sdk-python#433 to remove that code and add an update-and-signal-interleaved test.
What was changed
Updates are now sorted as equal with signals. IE: Order is preserved relative to signals, but both signals and updates are moved before other jobs.
Why?
It makes life easier for existing implementations, and specifically for languages that can't exert full control over when routines run (TS, mainly).
Checklist
Closes
How was this tested:
Existing tests
Any docs updates needed?